Fix long tuples in PatternLocalRewriter #47893

Youssef1313 · 2020-09-21T16:43:23Z

To explain the changes:

loweredInput.Arguments can have at max 8 arguments, with the 8th argument being another tuple. This is due to the available ValueTuples in the runtime:

I fixed the loop to have the exact number of items (which can be greater than 8), and introduced a static local function which gets the BoundExpression for the corresponding index.

When index < 7, just return Arguments[index].
When index >= 7, gets the corresponding TRest tuple, and apply the same logic for index - 7.

Youssef1313 · 2020-09-21T16:46:12Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

@@ -488,7 +488,7 @@ bool usesOriginalInput(BoundDecisionDagNode node)
                bool canShareInputs,
                out BoundExpression savedInputExpression)
            {
-                int count = loweredInput.Arguments.Length;
+                int count = loweredInput.Type.TupleElements.Length;


For tuple containing > 7 elements (10 elements for example), the previous value of count will be 8 (the TRest position) instead of 10, causing the loop after a few lines to run a fewer iteration than needed.

Youssef1313 · 2020-09-21T16:47:58Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

@@ -497,7 +497,7 @@ bool usesOriginalInput(BoundDecisionDagNode node)
                {
                    var field = loweredInput.Type.TupleElements[i].CorrespondingTupleField;
                    Debug.Assert(field != null);
-                    var expr = loweredInput.Arguments[i];
+                    var expr = getExpression(i, loweredInput);


Here, we can't just access Arguments[i] for two reasons:

If i == 8, the result expression will be a tuple, this is not intended.

If i > 8, this will throw an exception due to the other fix I made.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

333fred · 2020-09-21T19:57:18Z

@gafter if you are available, we'd appreciate a review 😄

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs

333fred · 2020-09-21T20:01:17Z

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs

+{   
+    public static void Main()
+    {
+        int x1 = (1, 1, 1, 1, 1, 1, 1, 1, 1, 1) switch


Please split out this test and verify the IL. I don't think we need to verify IL on all of these variations, but it would be good to have 1 of them verified.

@333fred I feel it's safer to verify the IL for different cases because in the past, the compiler had different behavior between the 8 elements case, and the > 8 elements case.

I'd at least verify the IL for 2 cases:

8 elements

Greater than 8 elements

333fred · 2020-09-21T20:01:39Z

Done review pass (commit 3).

333fred · 2020-09-21T20:02:47Z

Also, it looks like another test is failing with this change.

Youssef1313 · 2020-09-21T20:22:41Z

Also, it looks like another test is failing with this change.

I'll give it a look and try to fix that with the test changes you suggested.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs

jcouv

Done with review pass (iteration 3). Minor comments only

Youssef1313 · 2020-09-21T21:26:32Z

Also, it looks like another test is failing with this change.

With the change I introduced, GetArgumentRefKind method is now being called with indices > 7, which sounds correct to me. It just needs a similar logic to the static local function getExpression that I introduced. One small problem is where to define this logic to avoid duplicating it in two places.

Another problem is that I'm unsure of "who" calls GetArgumentRefKind. Is it used for things other than tuples (i.e. normal method parameters) in which case things may get a bit more complex (opened #47911 in a hope that test failures will answer).

Also I spotted an assertion that is now passing just "by luck":

        private void EmitArguments(ImmutableArray<BoundExpression> arguments, ImmutableArray<ParameterSymbol> parameters, ImmutableArray<RefKind> argRefKindsOpt)
        {
            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");
            Debug.Assert(parameters.All(p => p.RefKind == RefKind.None) || !argRefKindsOpt.IsDefault, "there are nontrivial parameters, so we must have argRefKinds");
            Debug.Assert(argRefKindsOpt.IsDefault || argRefKindsOpt.Length == arguments.Length, "if we have argRefKinds, we should have one for each argument");

            for (int i = 0; i < arguments.Length; i++)
            {
                RefKind argRefKind = GetArgumentRefKind(arguments, parameters, argRefKindsOpt, i);
                EmitArgument(arguments[i], argRefKind);
            }
        }

This assertion:

            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");

Assumes a max difference of 1 between arguments length and parameters length.

Because the corresponding test uses a tuple of 9 elements, and arguments is 8 (where the 8th argument is a tuple itself holding the 8th and 9th values). the assertion passes. But if it's a tuple of 10 elements. arguments.Length would still be 8 and the assertion will fail.

Given that, I'm now less confident with the change I made. It breaks some existing assumptions. I could just remove the assertion in EmitArguments, but I'm afraid this breaks something.

333fred · 2020-09-21T21:54:33Z

One small problem is where to define this logic to avoid duplicating it in two places.

Consider doing this as a static method on TupleTypeSymbol. I'll take a look at the code in EmitArguments a bit more.

Youssef1313 · 2020-09-21T22:06:24Z

One small problem is where to define this logic to avoid duplicating it in two places.

Consider doing this as a static method on TupleTypeSymbol. I'll take a look at the code in EmitArguments a bit more.

The existing code needs to get a BoundExpression from BoundObjectCreationExpression based on the index. I'm not sure if I can achieve the same if the extension method is on TupleTypeSymbol.

I found a class named BoundExpressionExtensions, looks like a good place for this extension method.

For the failing assertions, I think I may surround them with an if condition so they are executed for non-tuple parameters only.

333fred · 2020-09-21T22:45:38Z

            // We might have an extra argument for the __arglist() of a varargs method.
            Debug.Assert(arguments.Length == parameters.Length || arguments.Length == parameters.Length + 1, "argument count must match parameter count");

Perhaps the right thing to do is break this assert out into a conditional method that can verify the right thing. It would verify that:

arguments.Length == parameters.Length in the general case (and the last parameter is not an __arglist)
arguments.Length == parameters.Length + 1 when the last parameter is an __arglist
The tuple logic as appropriate.

You'll have to surround such a method if #if DEBUG blocks for now, since we have a bug with Conditional that isn't going to be fixed in the toolset until RC2 (iirc).

Youssef1313 · 2020-09-21T23:34:38Z

Actually I think I was incorrect when I said:

With the change I introduced, GetArgumentRefKind method is now being called with indices > 7, which sounds correct to me.

This isn't correct. ValueTuple constructor have at most 8 arguments. Even if the elements are greater than that, it doesn't make sense to call ArgumentRefKind with index > 7. So GetArgumentRefKind should be kept as it is with the assertion in it.

The callers & caller's assertion needs some tweaks. Or even I think the original fix shouldn't cascade like this.

RikkiGibson · 2020-09-22T18:06:32Z

GitHub keeps presenting my name at the top of the completion list, then the "remove all assignees" button pops in..

Youssef1313 · 2020-09-22T18:13:14Z

One thing to notice, when this was working in past, the generated code was using pointers in an unsafe context.

The change in this PR tries to make the code works again, but in non-unsafe fashion.

But I can't get it to work correctly.

@RikkiGibson @jcouv Are you aware of any PRs in the past that was around making the generated code non-unsafe?

RikkiGibson · 2020-09-22T18:18:18Z

@Youssef1313 I messaged you on gitter, let's figure out the answer to that and how to move forward.

RikkiGibson · 2020-09-22T20:04:33Z

Summary of the situation:

Pattern matching of tuples of more than 8 elements (in this scenario at least) has not worked for some time. At least not since the 16.5 time frame. For a while, pattern matching tuples with exactly 8 elements has worked, but later the semantics of the emitted code were broken.

Perhaps we should just look at the up-to-date implementation and try to fix it to work with indefinitely many elements, perhaps using a "too many locals" or similar diagnostic if we hit a meaningful limit.

Youssef1313 · 2020-09-22T21:34:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs

+        }
+
+        [Fact, WorkItem(47878, "https://github.com/dotnet/roslyn/issues/47878")]
+        public void VerifyIL_8ElementsTuple()


Is the IL for this test and the next test correct?

Youssef1313 · 2020-09-22T22:50:44Z

@RikkiGibson Looks like it should be working now. But my changes removed a whole array that used to exist, which makes me a little bit less confident. Can this change break some scenario that is currently not tested?

RikkiGibson · 2020-09-23T18:32:28Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs

                }

                var rewrittenDag = decisionDag.Rewrite(makeReplacement);
                savedInputExpression = loweredInput.Update(
-                    loweredInput.Constructor, arguments: newArguments.ToImmutableAndFree(), loweredInput.ArgumentNamesOpt, loweredInput.ArgumentRefKindsOpt,
+                    loweredInput.Constructor, arguments: loweredInput.Arguments, loweredInput.ArgumentNamesOpt, loweredInput.ArgumentRefKindsOpt,


I'm confused by the change to get rid of the newArguments array. I thought that would have undesired effects, particularly related to reuse of user temps or side effects.

@RikkiGibson I was expecting to see some test failures related to this too, but I don't know what scenarios exactly that should fail. Do you know any scenarios that should fail to add tests for them?

I tried the following scenario to induce a failure but it continued to have the expected side-effects in your branch:

[Fact, WorkItem(47878, "https://github.com/dotnet/roslyn/issues/47878")] public void VerifyIL_8ElementsTuple_SideEffects() { var text = @" using System; public class C { public static void Main() { int x = (M(1), M(2), M(3), M(4), M(5), M(6), M(7), M(8)) switch { (1, 2, 3, 4, 5, 6, 7, 7) => 1, (1, 2, 3, 4, 5, 6, 7, 8) => 1, _ => -1, }; Console.WriteLine(x); } static int M(int x) { Console.Write(x); return x; } } "; CompileAndVerify(text, expectedOutput: "123456781"); }

I think the next thing I would try would be to make many of the tuple components local variables and see if the lowering code reuses the local variables properly instead of introducing new ones.

I'll try to debug in more detail soon.

Here's the scenario where we have unexpected side effects:

[Fact, WorkItem(47878, "https://github.com/dotnet/roslyn/issues/47878")] public void VerifyIL_8ElementsTuple_SideEffects_02() { var text = @" using System; using System.Runtime.CompilerServices; public class C { public static void Main() { try { int x = (M(1), M(2), M(3), M(4), M(5), M(6), M(7), M(8)) switch { (1, 2, 3, 4, 5, 6, 7, 7) => 1, }; Console.WriteLine(x); } catch (SwitchExpressionException ex) { Console.Write(""💥""); } } static int M(int x) { Console.Write(x); return x; } } namespace System.Runtime.CompilerServices { public class SwitchExpressionException : InvalidOperationException { public SwitchExpressionException() {} public SwitchExpressionException(object unmatchedValue) => UnmatchedValue = unmatchedValue; public object UnmatchedValue { get; } } } "; CompileAndVerify(text, expectedOutput: "12345678💥"); }

With your change, the actual output is 1234567812345678💥. Let's revert that part of the change.

@RikkiGibson That will get us to the assertion failure too. I'm still not sure what is the appropriate fix. I'll add this test and give it a try again, but not sure if I would be able to get the correct fix.

I think the problem is that when we "unwrap" the tuple elements as is done in this PR, the length of the arguments no longer matches the length of the arguments ot the top-level object creation node. i.e. loweredInput.Arguments.Length should never be more than 8, and tuples of size 8 or larger must nest their arguments in additional object creation nodes.

I can try and look a bit to see how to do this--the fix isn't coming to mind instantly. But to get the behavior of the scenario fixed in 16.8 we may want to take #48028 and then optimize it later.

RikkiGibson · 2020-09-25T22:48:44Z

I will move this back to draft status. Feel free to mark it ready for review again if you want us to take another look later.

Youssef1313 · 2021-04-03T10:18:09Z

Closing this stale PR.

Fix long tuples in PatternLocalRewriter

995012f

Youssef1313 requested a review from a team as a code owner September 21, 2020 16:43

Youssef1313 commented Sep 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs Outdated Show resolved Hide resolved

Youssef1313 marked this pull request as draft September 21, 2020 17:24

Youssef1313 marked this pull request as ready for review September 21, 2020 17:24

Youssef1313 marked this pull request as draft September 21, 2020 17:25

Youssef1313 marked this pull request as ready for review September 21, 2020 17:25

Youssef1313 added 2 commits September 21, 2020 19:30

More tests

e281d60

Fix test

3ed21a1

333fred reviewed Sep 21, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs Outdated Show resolved Hide resolved

333fred reviewed Sep 21, 2020

View reviewed changes

jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 21, 2020

jcouv self-assigned this Sep 21, 2020

jcouv reviewed Sep 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs Outdated Show resolved Hide resolved

jcouv reviewed Sep 21, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs Show resolved Hide resolved

jcouv reviewed Sep 21, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/SwitchTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Sep 21, 2020

View reviewed changes

RikkiGibson unassigned jcouv Sep 22, 2020

RikkiGibson assigned RikkiGibson and jcouv Sep 22, 2020

Youssef1313 added 3 commits September 22, 2020 20:44

Some feedback, and hopefully fix the logic

c921f92

Feedback

d31ab2b

Add IL tests for long tuples

de76513

Youssef1313 commented Sep 22, 2020

View reviewed changes

Fix test

33f4e5d

This was referenced Sep 23, 2020

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Tracking issue in roslyn-ci jaredpar/runfo#44

Closed

RikkiGibson reviewed Sep 23, 2020

View reviewed changes

RikkiGibson mentioned this pull request Sep 24, 2020

Don't optimize large tuple literal switch inputs #48028

Merged

RikkiGibson marked this pull request as draft September 25, 2020 22:48

RikkiGibson removed their assignment Oct 5, 2020

Base automatically changed from master to main March 3, 2021 23:52

Youssef1313 closed this Apr 3, 2021

Youssef1313 deleted the fix-long-tuples branch April 3, 2021 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix long tuples in PatternLocalRewriter #47893

Fix long tuples in PatternLocalRewriter #47893

Youssef1313 commented Sep 21, 2020 •

edited

Loading

Youssef1313 Sep 21, 2020

Youssef1313 Sep 21, 2020

333fred commented Sep 21, 2020

333fred Sep 21, 2020

Youssef1313 Sep 22, 2020

333fred commented Sep 21, 2020

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020

jcouv left a comment

Youssef1313 commented Sep 21, 2020 •

edited

Loading

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020 •

edited

Loading

RikkiGibson commented Sep 22, 2020

Youssef1313 commented Sep 22, 2020

RikkiGibson commented Sep 22, 2020

RikkiGibson commented Sep 22, 2020

Youssef1313 Sep 22, 2020

Youssef1313 commented Sep 22, 2020

RikkiGibson Sep 23, 2020

Youssef1313 Sep 23, 2020

RikkiGibson Sep 23, 2020

RikkiGibson Sep 23, 2020

Youssef1313 Sep 23, 2020

RikkiGibson Sep 24, 2020

RikkiGibson commented Sep 25, 2020

Youssef1313 commented Apr 3, 2021

Fix long tuples in PatternLocalRewriter #47893

Fix long tuples in PatternLocalRewriter #47893

Conversation

Youssef1313 commented Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Sep 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Sep 21, 2020

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020

jcouv left a comment

Choose a reason for hiding this comment

Youssef1313 commented Sep 21, 2020 • edited Loading

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020

333fred commented Sep 21, 2020

Youssef1313 commented Sep 21, 2020 • edited Loading

RikkiGibson commented Sep 22, 2020

Youssef1313 commented Sep 22, 2020

RikkiGibson commented Sep 22, 2020

RikkiGibson commented Sep 22, 2020

Choose a reason for hiding this comment

Youssef1313 commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Sep 25, 2020

Youssef1313 commented Apr 3, 2021

Youssef1313 commented Sep 21, 2020 •

edited

Loading

Youssef1313 commented Sep 21, 2020 •

edited

Loading

Youssef1313 commented Sep 21, 2020 •

edited

Loading